Skip to content

Python: fix(python): prevent local tool approvals from serializing as MCP responses - #8088

Open
pratik wayase (PratikWayase) wants to merge 4 commits into
microsoft:mainfrom
PratikWayase:fix/local-tool-approval-clean
Open

Python: fix(python): prevent local tool approvals from serializing as MCP responses#8088
pratik wayase (PratikWayase) wants to merge 4 commits into
microsoft:mainfrom
PratikWayase:fix/local-tool-approval-clean

Conversation

@PratikWayase

@PratikWayase pratik wayase (PratikWayase) commented Sep 5, 2026

Copy link
Copy Markdown
Contributor

Motivation & Context

Approved local (non-MCP) tool calls fail with a hard 400 error from the OpenAI Responses API when using approval_mode="always_require" with service-side storage. This blocks any agent that relies on local gated tools from completing their turn.

The bug requires two independent defects to manifest, and fixing either prevents the 400. This PR addresses both to ensure robust, order-independent behavior.

Description & Review Guide

  • What are the major changes?

    1. OpenAI Serializer (_chat_client.py): Added an _is_hosted_tool_approval guard to both function_approval_request and function_approval_response arms in _prepare_content_for_openai. Local approvals now return {} and are safely filtered out upstream. Also restored the request_uses_service_side_storage guard on the response arm to prevent orphaned responses under storage.
    2. Approval Control Filtering (_sessions.py, _tools.py): Replaced the fragile, order-dependent single-pass queue-popping logic with a robust two-pass, set-based approach across four functions: _approval_controls_to_keep, _collect_approval_responses, _collect_unanswered_approval_requests, and _remove_unanswered_approval_batches_from_model_input. Pass 1 collects all resolved call_ids regardless of position; Pass 2 filters using the pre-built set. This eliminates the ordering bug where a terminal result appearing before an approval response (the "approval-resume" layout) caused the resolved response to leak into model input.
    3. Tests: Updated existing tests that previously encoded the broken behavior. Added comprehensive regression tests covering order-independent resolution, local vs. hosted serialization boundaries, follow-up request semantics, and service-side storage symmetry.

  • What is the impact of these changes?

    • User-facing: Local gated tools now work correctly on the Responses API path without throwing 400 errors.
    • Reliability: Approval resume flows no longer produce errors regardless of message ordering (e.g., result before response).
    • Compatibility: Hosted/MCP tool approvals are completely unaffected and continue to serialize and flow through the API as before.
    • No breaking changes: All modifications are strictly internal filtering/serialization logic.

  • What do you want reviewers to focus on?

    • The two-pass pattern in _approval_controls_to_keep — verify the semantic boundary between what session history filtering resolves (terminal results + matching responses) vs. what current-turn collectors resolve.
    • The _is_hosted_tool_approval guard placement in _chat_client.py — confirm returning {} for local approvals is safe given all upstream callers use if prepared:.
    • The updated existing tests — these previously encoded buggy contracts; verify the new assertions match the intended correct behavior.

Related Issue

Fixes #7452

related PR #7473

Contribution Checklist

  • The code builds clean without any errors or warnings
  • All unit tests pass, and I have added new tests where possible
  • The PR follows the Contribution Guidelines
  • This PR is linked to an issue and there is no other open PR for this issue.
  • This is not a breaking change.

Copilot AI balanced review requested due to automatic review settings September 5, 2026 10:29
@agent-framework-automation agent-framework-automation Bot added documentation Usage: [Issues, PRs], Target: documentation in the code base and learn docs python Usage: [Issues, PRs], Target: Python labels Sep 5, 2026
@github-actions github-actions Bot changed the title fix(python): prevent local tool approvals from serializing as MCP responses Python: fix(python): prevent local tool approvals from serializing as MCP responses Sep 5, 2026
Copilot AI reviewed Sep 5, 2026
View reviewed changes

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Approval correlation mishandles cross-message ordering and reused call IDs, risking duplicate execution or lost pending approvals.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Fixes local approval leakage into OpenAI MCP payloads and makes approval filtering order-independent.

Changes:

  • Restricts MCP serialization to hosted approvals.
  • Reworks approval resolution and filtering.
  • Adds regression coverage for approval ordering and serialization.
File summaries
File Description
python/packages/openai/agent_framework_openai/_chat_client.py Guards hosted approval serialization.
python/packages/openai/tests/openai/test_openai_chat_client.py Tests local and hosted serialization behavior.
python/packages/core/agent_framework/_tools.py Reworks approval collection and filtering.
python/packages/core/agent_framework/_sessions.py Makes history filtering order-independent.
python/packages/core/tests/core/test_tools.py Adds approval filtering regressions.
python/packages/core/tests/core/test_sessions.py Tests history approval resolution.
python/packages/core/tests/core/test_agent_executor.py Formatting-only cleanup.
python/packages/core/tests/core/test_function_invocation_logic.py Formatting-only cleanup.
docs/specs/004-python-function-calling-loop.md Formatting-only specification edits.
Review details

Suppressed comments (1)

python/packages/core/agent_framework/_tools.py:2447

  • This global call-id check marks a fresh pending request as answered if any older occurrence with the same call_id has a terminal result. _remove_unanswered_approval_batches_from_model_input then fails to recognize and hide that pending batch, even though call-id reuse after completion is supported (python/packages/core/AGENTS.md:173-179). Pair terminal events with request occurrences instead of applying them to every request sharing the id.
                if content.id in answered_approval_ids or function_call.call_id in resolved_call_ids:
                    continue
  • Files reviewed: 9/9 changed files
  • Comments generated: 3
  • Review effort level: Balanced

💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.

Comment thread python/packages/core/agent_framework/_sessions.py Outdated
Comment thread python/packages/core/agent_framework/_tools.py Outdated
Comment thread python/packages/openai/tests/openai/test_openai_chat_client.py Outdated
moonbox3
Evan Mattson (moonbox3) reviewed Sep 7, 2026
View reviewed changes
Comment thread python/packages/core/agent_framework/_tools.py Outdated
Comment thread python/packages/core/agent_framework/_tools.py Outdated
@PratikWayase

pratik wayase (PratikWayase) commented Sep 9, 2026

Copy link
Copy Markdown
Contributor Author

Evan Mattson (@moonbox3) The latest push fixes the CI failures.
I also made sure to run the aggregate test suite locally this time uv run poe check and uv run poe test -P core to ensure there were no cross-package blind spots. Everything passes locally now!

Could you please re-run the failed checks or let me know if there's anything else to address? Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@moonbox3 Evan Mattson (moonbox3) moonbox3 left review comments
Copilot code review Copilot
Copilot review effort, defaults to Balanced
Applies to this pull request for everyone.Learn more about Copilot code review.
Copilot left review comments
@chetantoshniwal chetantoshniwal Awaiting requested review from chetantoshniwal chetantoshniwal is a code owner
@eavanvalkenburg Eduard van Valkenburg (eavanvalkenburg) Awaiting requested review from eavanvalkenburg eavanvalkenburg is a code owner
@TaoChenOSU Tao Chen (TaoChenOSU) Awaiting requested review from TaoChenOSU TaoChenOSU is a code owner
@jpalvarezl Jose Alvarez (jpalvarezl) Awaiting requested review from jpalvarezl jpalvarezl is a code owner
@giles17 Giles Odigwe (giles17) Awaiting requested review from giles17 giles17 is a code owner
@westey-m westey (westey-m) Awaiting requested review from westey-m westey-m is a code owner
@peibekwe Peter Ibekwe (peibekwe) Awaiting requested review from peibekwe peibekwe is a code owner
@baywet Vincent Biret (baywet) Awaiting requested review from baywet baywet is a code owner
@rogerbarreto Roger Barreto (rogerbarreto) Awaiting requested review from rogerbarreto rogerbarreto is a code owner
@SergeyMenshykh SergeyMenshykh Awaiting requested review from SergeyMenshykh SergeyMenshykh is a code owner

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

documentation Usage: [Issues, PRs], Target: documentation in the code base and learn docs python Usage: [Issues, PRs], Target: Python

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Python: [Bug]: A local tool's approval response is serialized to the Responses API as an MCP approval response, with no matching request → 400

3 participants

@PratikWayase @moonbox3